Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flow: add loki.source.azure_event_hubs component #3412

Merged
merged 17 commits into from Apr 6, 2023

Conversation

akselleirv
Copy link
Contributor

@akselleirv akselleirv commented Apr 3, 2023

PR Description

Add a new component loki.source.azure_event_hubs which read messages from Azure Event Hub using Kafka and forwards them to other loki components. It is a port of this PR: grafana/loki#8787 with the addition of OAUTHBEARER authentication.

The main motivation behind this PR is adding support for collecting logs from Azure resources.

This authentication mechanism is also added to the loki.source.kafka component as this component depends on it.

Which issue(s) this PR fixes

Not related to an issue, but was created based on this conversation: https://grafana.slack.com/archives/C01050C3D8F/p1680105638704069

Notes to the Reviewer

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2023

CLA assistant check
All committers have signed the CLA.

@akselleirv akselleirv force-pushed the feat/azureeventhubs branch 6 times, most recently from eeb9da8 to bd6a9f9 Compare April 3, 2023 09:54
@rfratto
Copy link
Member

rfratto commented Apr 3, 2023

❤️ Thanks for the PR! We'll review this as soon as we can.

Copy link
Member

@tpaschalis tpaschalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! 🙌 Comments are mostly around documentation and style, this is looking good!

component/all/all.go Outdated Show resolved Hide resolved
component/loki/source/azureeventhubs/azureeventhubs.go Outdated Show resolved Hide resolved
case <-ctx.Done():
return nil
case entry := <-c.handler:
c.mut.RLock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for @tpaschalis: we might be susceptible to something like #3391 here, both for this and the loki.source.kafka component.

Will check it out to see if there's a need for a fix after this is merged.

Comment on lines 142 to 144
t, err := kt.NewSyncer(c.opts.Registerer, c.opts.Logger, cfg, entryHandler, &parser.AzureEventHubsTargetMessageParser{
DisallowCustomMessages: newArgs.DisallowCustomMessages,
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers, here's the crucial bit: the shared kafkatarget package was updated in Promtail to expect a Parser implementation, and they've added one for Azure Event Hubs.

@akselleirv
Copy link
Contributor Author

Nice one! 🙌 Comments are mostly around documentation and style, this is looking good!

Thank you for the review @tpaschalis! I've updated the PR based on your feedback

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Nice! I reviewed this purely from the documentation perspective, and I'll let @tpaschalis sign off on the code side.

@akselleirv
Copy link
Contributor Author

🎉 Nice! I reviewed this purely from the documentation perspective, and I'll let @tpaschalis sign off on the code side.

Thank you! Updated the changes now :)

rfratto added a commit that referenced this pull request Apr 4, 2023
Remove non-recommended additional header as pointed out here: #3412 (comment)
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @tpaschalis I'll leave final approval / merging to you :)

rfratto added a commit that referenced this pull request Apr 4, 2023
Remove non-recommended additional header as pointed out here: #3412 (comment)
grafanabot pushed a commit that referenced this pull request Apr 4, 2023
Remove non-recommended additional header as pointed out here: #3412 (comment)

(cherry picked from commit e2c8f54)
rfratto added a commit that referenced this pull request Apr 4, 2023
Remove non-recommended additional header as pointed out here: #3412 (comment)

(cherry picked from commit e2c8f54)

Co-authored-by: Robert Fratto <robertfratto@gmail.com>
@tpaschalis
Copy link
Member

tpaschalis commented Apr 5, 2023

@akselleirv apologies for the hassle, but we recently got a PR that mass updates files for typos, grammatical errors etc. Could you please rebase, and I think we're good to go!

akselleirv and others added 8 commits April 5, 2023 22:15
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
@akselleirv
Copy link
Contributor Author

@tpaschalis rebased now and the CI is all green 🚀

Copy link
Member

@tpaschalis tpaschalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you Aksel!

@tpaschalis tpaschalis merged commit c4ce55a into grafana:main Apr 6, 2023
6 checks passed
@akselleirv akselleirv deleted the feat/azureeventhubs branch April 6, 2023 06:35
@leelax22
Copy link

I have a question about azure event hub.

grafana/loki#8788

In the following link
"Messages contain a few(one or more) log lines. We need to be able to split them into separate entries."

This is the part I was struggling with, so I tried using loki.source.azure_event_hubs, hoping that it would allow me to split them into separate logs.

The logs I collected through my azure aks diagnostics setup come into loki via event hub and grafana agent flow as follows.
image

Looking at the code change history on github, it seems that tests were made through log data from function app or logic app, etc. but is the seperate entry applied only to those items? Or is there something I'm missing?

@leelax22
Copy link

I solved the problem. The problem was loki api address is not 'url = "loki:3100/api/v1/push" in the docs (https://grafana.com/docs/agent/latest/flow/reference/components/loki.source.azure_event_hubs/#example). It should be "loki:3100/loki/api/v1/push" accroding to the docs(https://grafana.com/docs/loki/latest/api/)

clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
Remove non-recommended additional header as pointed out here: #3412 (comment)
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
Remove non-recommended additional header as pointed out here: #3412 (comment)
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants